Fix phpstan/phpstan#7858: Redundant type casting drops next expression from condition and switches the context#5080
Conversation
911404a to
bf249cc
Compare
staabm
left a comment
There was a problem hiding this comment.
fix the implementation to obey asserted types of newly added tests
|
Both background tasks have completed successfully. The fix has been committed and pushed. |
- Unwrap redundant cast expressions in Smaller/SmallerOrEqual type specifying so that narrowing propagates to the inner variable instead of the cast expr - When a cast produces the same type as its inner expression (e.g. (int) on an int variable), the narrowing is now applied to the original variable - New regression test in tests/PHPStan/Analyser/nsrt/bug-7858.php Closes phpstan/phpstan#7858
The previous approach only unwrapped cast expressions when the cast type equaled the inner type (redundant cast). This failed for cases where the variable had a union type with non-integer parts (e.g. int|numeric-string after ctype_digit), because the cast was not redundant and the narrowing was never applied. The narrowing type (e.g. mixed~(0.0|int<min, 2021>|false|null)) already correctly preserves non-integer parts of the type through intersection, so unconditional unwrapping produces correct results for all cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
940faf7 to
b769b67
Compare
| $rightExpr = $expr->right; | ||
| if ($rightExpr instanceof Expr\Cast) { | ||
| $rightExpr = $rightExpr->expr; | ||
| } |
There was a problem hiding this comment.
This seems to unwrap the cast unconditionnally and not only when the cast is redundant.
Wouldn't this introducing regression for non redundant cast ?
Add an assertType check on (int) $year in all the written example.
There was a problem hiding this comment.
Pushed. Here's a summary of the changes:
TypeSpecifier.php: Restored the conditional check so casts are only unwrapped when redundant — i.e., when $scope->getType($castExpr)->equals($scope->getType($castExpr->expr)). This prevents incorrect type narrowing when the cast is meaningful (e.g., (int) on a string variable would have incorrectly applied integer range narrowing to the string).
bug-7858.php test updates:
- Added
assertTypefor(int) $year/(int) $mixedin all test functions — all correctly inferint<2022, max>(the cast expression itself gets narrowed) foo(int $year):$yearstill correctly narrows toint<2022, max>(cast is redundant, unwrapped)baz($year)andbam(int|string $year):$yeartype updated toint<48, 57>|int<256, max>|numeric-string(cast is non-redundant, not unwrapped, so the comparison narrowing applies to the cast expression rather than the variable)ban(string $year): unchanged —numeric-string(cast non-redundant, string not narrowed by int comparison)bak($mixed):$mixedtype updated tofloat|int|numeric-string(cast non-redundant)
All 11,904 tests pass and PHPStan reports no errors.
The cast unwrapping in Smaller/SmallerOrEqual type specifying was unconditionally unwrapping all casts, which could cause incorrect type narrowing when the cast is not redundant (e.g. (int) on a string variable). Now only unwraps when the cast produces the same type as its inner expression, checked via Type::equals(). Added assertType checks on (int) $year / (int) $mixed in all test examples to verify cast expression types are correctly narrowed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
left a comment
There was a problem hiding this comment.
For type which are both int OR something else, we could improve inference ;
For instance the current test is
function bam(int|string $year): void
{
if (!ctype_digit($year) || (int)$year < 2022) {
throw new \RuntimeException();
}
assertType('int<48, 57>|int<256, max>|numeric-string', $year);
assertType('int<2022, max>', (int) $year);
}
but we could have
function bam(int|string $year): void
{
if (!ctype_digit($year) || (int)$year < 2022) {
throw new \RuntimeException();
}
assertType('int<2022, max>|numeric-string', $year);
assertType('int<2022, max>', (int) $year);
}
When a cast like (int) is used in a comparison on a union type (e.g., int|string), the integer members where the cast is identity now get narrowed by the comparison, while non-integer members are preserved. For example, `(int)$year < 2022` on `int|string` now correctly narrows the int part to `int<2022, max>` while keeping the string part unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's a summary of the changes: TypeSpecifier.php: Added support for partially redundant casts in comparison type narrowing. When a cast expression like
The narrowed type for the inner expression is computed as Test updates (
All 11,904 tests pass and PHPStan reports no errors. |
|
at this point I think it is too much code for the value given thru the implementation. would close the PR than. @VincentLanglet wdyt? |
|
Agree |
Summary
When a redundant type cast like
(int)was used on a variable inside a comparison within a boolean OR condition, PHPStan dropped the type narrowing from the comparison and reported the wrong type. For example,if (!ctype_digit($year) || (int) $year < 2022)should narrow$yeartoint<2022, max>after the if-throw, but instead reportedint<48, 57>|int<256, max>(only the ctype_digit narrowing, ignoring the< 2022comparison entirely).Changes
src/Analyser/TypeSpecifier.phpin theSmaller/SmallerOrEqualhandling sectionType::equals()), the inner expression is used for type narrowing instead of the cast expression$year) rather than the transient cast expression ((int) $year) which has no trackable identity in the scopeRoot cause
The
TypeSpecifier::specifyTypesInCondition()method creates type specifications keyed by expression string. When the left side of<was(int) $year, the narrowing was created for the expression string(int) $year. However, the scope only tracks the variable$year, so the narrowing for(int) $yearwas never matched and applied. When the cast is redundant (the inner expression already has the target type), the cast should be unwrapped so the narrowing targets the actual variable.Test
Added
tests/PHPStan/Analyser/nsrt/bug-7858.phpwith two test functions:(int)cast in the condition (the buggy case) - assertsint<2022, max>int<2022, max>Fixes phpstan/phpstan#7858